Fix GH-20370: forbid user stream filters to violate typed property constraints #20373
Conversation
alexandre-daubois
force-pushed
the
user-stream-filter-props
branch
from
b0808ce to
786ad32
Compare
alexandre-daubois
force-pushed
the
user-stream-filter-props
branch
from
786ad32 to
2ad8c64
Compare
ndossche
left a comment
ndossche
left a comment
There was a problem hiding this comment.
On a phone , so can't check. But you're looking at the property info now, however dynamic properties don't have any. So that means that a dynamically created property $stream would've been overwritten before this patch but now no longer. So that is an unintended behaviour change.
|
Oh alright, I wasn't aware of this subtlety of dynamic properties. Let's refine that. |
|
Hmm so that would require to someone create the dynamic property in filter first and it could then be used in subsequent calls for stream, right? If so, that sounds like a proper edge case... :) |
|
The easiest way to test this is to create a onCreate method that sets a dynamic property on $this |
alexandre-daubois
force-pushed
the
user-stream-filter-props
branch
from
2ad8c64 to
fe4961a
Compare
|
This will work, except for a pre-existing bug: It would also be great to try to avoid OBJPROP so that the property table isn't rebuilt, but that isn't critical. |
|
I tried to implement what you have in mind in 04e0955 |
alexandre-daubois
force-pushed
the
user-stream-filter-props
branch
2 times, most recently
from
7208c60 to
04e0955
Compare
alexandre-daubois
force-pushed
the
user-stream-filter-props
branch
from
04e0955 to
3d5358f
Compare
alexandre-daubois
force-pushed
the
user-stream-filter-props
branch
from
314fde5 to
c838191
Compare
ndossche
left a comment
ndossche
left a comment
There was a problem hiding this comment.
Thanks for tackling this. As I said in the issue I opened, it's a bit of an annoying issue (as you have felt).
| if (Z_OBJ_HT_P(obj)->has_property(Z_OBJ_P(obj), stream_name, ZEND_PROPERTY_EXISTS, NULL)) { | ||
| zval null_zval; | ||
| ZVAL_NULL(&null_zval); | ||
| zend_update_property_ex(Z_OBJCE_P(obj), Z_OBJ_P(obj), stream_name, &null_zval); |
There was a problem hiding this comment.
You can use zend_update_property_null.
|
|
||
| zend_string *stream_name = ZSTR_INIT_LITERAL("stream", 0); | ||
| if (Z_OBJ_HT_P(obj)->has_property(Z_OBJ_P(obj), stream_name, ZEND_PROPERTY_EXISTS, NULL)) { | ||
| zval stream_zval; |
There was a problem hiding this comment.
A good follow-up for master would be to clean up the duplication of the error branches.
(You don't need to do anything with this comment in this PR, this is just a note for future work)
| #include "ext/standard/basic_functions.h" | ||
| #include "ext/standard/file.h" | ||
| #include "ext/standard/user_filters_arginfo.h" | ||
| #include "zend_exceptions.h" |
There was a problem hiding this comment.
Why did you need this include?
| EG(fake_scope) = Z_OBJCE_P(obj); | ||
|
|
||
| zend_string *stream_name = ZSTR_INIT_LITERAL("stream", 0); | ||
| if (Z_OBJ_HT_P(obj)->has_property(Z_OBJ_P(obj), stream_name, ZEND_PROPERTY_EXISTS, NULL)) { |
There was a problem hiding this comment.
I'd be tempted to use a cache slot anyway as you repeat the check below, not critical though.
Alternatively: The previous code remembered whether the property existed, perhaps you can do this too, but it's not critical. This alternative idea seems clean as it would simplify the code a bit further.
| php_error_docref(NULL, E_WARNING, "Unprocessed filter buckets remaining on input brigade"); | ||
| } | ||
| zend_string_release(stream_name); | ||
| EG(fake_scope) = old_scope; |
There was a problem hiding this comment.
I think you need to restore the fake scope prior to emitting the warning such that potential error handlers can't utilise the fake scope (not sure).
5 participants
Use
zend_update_property()instead of doing things "manually".